nrf_rpc: Add function for single group initialization #1593

Vge0rge · 2024-11-27T13:48:54Z

Adds the function nrf_rpc_init_group to allow initializing an nrf_rpc group individually. This is useful when groups need to be initialized in different boot stages.

nrf_rpc_init now takes into account that a group could could be initialized using the nrf_rpc_init_group and avoids initializiting again.

doki-nordic · 2024-12-10T12:35:02Z

nrf_rpc/nrf_rpc.c

+	return 0;
+}
+
+int nrf_rpc_init(nrf_rpc_err_handler_t err_handler)


The err_handler is never assigned to global handler.

Good catch, I changed that.

doki-nordic · 2024-12-10T13:22:43Z

nrf_rpc/nrf_rpc.c

+static void default_err_handler(const struct nrf_rpc_err_report *report)
+{
+	NRF_RPC_ERR("nRF RPC error %d ocurred. See nRF RPC logs for more details", report->code);
+	k_oops();


This should be OS-independent code. k_oops() is a Zephyr function. Please pass it through OS porting layer.

Regarding this, do you refer to this OS porting layer do you refer to the nrf_rpc one?
So what you are proposing is to add another function here:
https://github.com/nrfconnect/sdk-nrf/blob/main/subsys/nrf_rpc/nrf_rpc_os.c

Which handles the error case?

Do you have an opinion on this Pawel? I can do what I am describing here if you have the same understanding as me

reviewer is away. nitpicks were addressed

pdunaj · 2024-12-11T16:15:52Z

nrf_rpc/nrf_rpc.c


-		NRF_RPC_ASSERT(transport != NULL);
+	if(group->data->transport_initialized) {


space missing
s/if(/if (

pdunaj · 2024-12-11T16:17:16Z

nrf_rpc/nrf_rpc.c

+	for (NRF_RPC_AUTO_ARR_FOR(iter, group, &nrf_rpc_groups_array,
+				 const struct nrf_rpc_group)) {
+
+		transport_init_single(receive_cb, group);


error is not checked

This is intentional though, because I saw in the previous implementation they didn't return an error when a single initialization failed.
The logic here:

sdk-nrfxlib/nrf_rpc/nrf_rpc.c

Line 420 in 4929937

for (NRF_RPC_AUTO_ARR_FOR(iter, group, &nrf_rpc_groups_array,

Should be the same with the logic in the function transport_init_single basically.

The error that I return in this function is the logic which exists here:

sdk-nrfxlib/nrf_rpc/nrf_rpc.c

Line 458 in 4929937

if (waiting_group_count > 0) {

pdunaj · 2024-12-11T16:17:46Z

nrf_rpc/nrf_rpc.c

@@ -1144,20 +1162,67 @@ int nrf_rpc_init(nrf_rpc_err_handler_t err_handler)
 		return err;
 	}

-	for (i = 0; i < CONFIG_NRF_RPC_CMD_CTX_POOL_SIZE; i++) {
+	for (int i = 0; i < CONFIG_NRF_RPC_CMD_CTX_POOL_SIZE; i++) {


s/int/size_t

pdunaj · 2024-12-11T16:19:19Z

nrf_rpc/nrf_rpc.c

+int nrf_rpc_init_group(const struct nrf_rpc_group *group)
+{
+	int err = nrf_rpc_prepare_init();
+	if(err < 0) {


missing space if (

pdunaj · 2024-12-11T16:19:52Z

nrf_rpc/nrf_rpc.c

+	NRF_RPC_DBG("Initializing nRF RPC module");
+
+	err = nrf_rpc_prepare_init();
+	if(err < 0) {


pdunaj · 2024-12-11T16:24:24Z

I assume that @doki-nordic have seen no problems with the actual change.
See few style issues to fix but the biggest problem is that error is ignored when transport is initialized.

pdunaj · 2024-12-12T19:13:13Z

Hi @Vge0rge , please just fix the error handling and I will approve and merge.

frkv

LGTM assuming the k_oops abstraction is handled in follow-up work

nrf_rpc/nrf_rpc.c

Damian-Nordic · 2025-03-27T09:50:34Z

nrf_rpc/nrf_rpc.c

+	}
+
+	NRF_RPC_ASSERT(group_count > 0);
+	if (initialized_group_count == group_count - 1) {


Shouldn't you use waiting_group_count? If there are groups without NRF_RPC_FLAGS_WAIT_ON_INIT flag, the wait below will hang indefinitely. By the way, it's non-obvious that the function waits only if there's only one group that has not been initialized so far, so this should be documented or there should be a follow up filed to make the group initialization less coupled (so that each group can only wait for its own initialization completion).

What I was trying to do here to here is to call the event_wait function only for the last group which was getting initialized. The function groups_init_event_wait checks for waiting threads so it will actually not hang. But on the other hand now that I look at it again there is a bug in this indeed. The initialized_group_count only increases for the threads which have the NRF_RPC_FLAGS_WAIT_ON_INIT flag enabled, so if there is one thread that doesn't then the wait will never get called. I will have to refactor this.

Ok, so another issue is that if you have two groups and you execute nrf_rpc_init_group for both of them without a delay, you may end up in a situation that neither call will enter this if (because init packet for the first group may not arrive before you execute the second nrf_rpc_init_group). So my concern is that if the goal of this function is to make sure that all groups are initialized after you call this function for the last group, you may be surprised in some scenarios :).

Damian-Nordic · 2025-03-27T09:52:06Z

nrf_rpc/nrf_rpc.c

+
+int nrf_rpc_init_group(const struct nrf_rpc_group *group)
+{
+	int err = nrf_rpc_prepare_init();


It seems that this code is not thread-safe. Is there an assumption that nrf_rpc_init_group must be called from the main thread only? If so, I think we should document it. Otherwise, nrf_rpc_prepare_init may end up being executed from two threads.

You are right that this is not thread safe, but is it not the case that the whole nrf_pc is not thread safe? The variable is_initialized was static before, I didn't change that. I am not too familiar with the library but it I see many static objects being manipulated so I thought that it is not thread safe. Am I missing something?

Calling nRF RPC commands is thread-safe as far as I understand but the init is not. @doki-nordic has more background but my impression is that the authors originally assumed that that the init would be called once. If we now allow more flexibility it becomes harder to enforce this, especially if this is not documented.

Vge0rge · 2025-03-28T14:51:52Z

Converted to draft until I finish the refactoring to signal that this should get merged as is.

Adds the function nrf_rpc_init_group to allow initializing an nrf_rpc group individually. This is useful when groups need to be initialized in different boot stages. nrf_rpc_init now takes into account that a group could could be initialized using the nrf_rpc_init_group and avoids initializiting again. Signed-off-by: Georgios Vasilakis <georgios.vasilakis@nordicsemi.no>

Vge0rge requested a review from doki-nordic as a code owner November 27, 2024 13:48

NordicBuilder mentioned this pull request Nov 27, 2024

manifest: sdk-nrfxlib: nrf_prc: Add function for single group initialization nrfconnect/sdk-nrf#19098

Closed

Vge0rge changed the title ~~nrf_prc: Add function for single group initialization~~ nrf_rpc: Add function for single group initialization Nov 27, 2024

Vge0rge force-pushed the 54h20_psa_rng branch from a943a29 to cd4ad91 Compare November 27, 2024 13:53

NordicBuilder mentioned this pull request Nov 27, 2024

entropy: Add PSA rng as the entropy provider for the nrf54h20 nrfconnect/sdk-nrf#17200

Open

Vge0rge force-pushed the 54h20_psa_rng branch 3 times, most recently from 1351882 to 7a4cfd2 Compare December 4, 2024 22:13

Vge0rge force-pushed the 54h20_psa_rng branch 2 times, most recently from 6d9476d to 326b004 Compare December 9, 2024 08:42

doki-nordic requested changes Dec 10, 2024

View reviewed changes

doki-nordic previously requested changes Dec 10, 2024

View reviewed changes

Vge0rge force-pushed the 54h20_psa_rng branch from 326b004 to fa5ea93 Compare December 11, 2024 09:35

pdunaj requested changes Dec 11, 2024

View reviewed changes

Vge0rge force-pushed the 54h20_psa_rng branch 2 times, most recently from fefd809 to d078aad Compare December 19, 2024 16:07

pdunaj approved these changes Dec 19, 2024

View reviewed changes

frkv approved these changes Dec 20, 2024

View reviewed changes

Vge0rge force-pushed the 54h20_psa_rng branch from d078aad to 2873f50 Compare December 23, 2024 11:08

Vge0rge force-pushed the 54h20_psa_rng branch 3 times, most recently from a6ab60d to 905cc1a Compare January 13, 2025 11:28

Vge0rge force-pushed the 54h20_psa_rng branch from 905cc1a to d727227 Compare January 14, 2025 15:13

Vge0rge force-pushed the 54h20_psa_rng branch 2 times, most recently from d192102 to 279e37c Compare February 21, 2025 13:48

Vge0rge force-pushed the 54h20_psa_rng branch 2 times, most recently from cb90371 to fc46274 Compare March 4, 2025 08:53

Vge0rge force-pushed the 54h20_psa_rng branch from fc46274 to 679b601 Compare March 12, 2025 14:12

NordicBuilder mentioned this pull request Mar 12, 2025

entropy: 54h20 PSA RNG fixes nrfconnect/sdk-nrf#20896

Merged

Vge0rge force-pushed the 54h20_psa_rng branch 2 times, most recently from 7605dd9 to a8ea4c0 Compare March 25, 2025 13:25

Vge0rge force-pushed the 54h20_psa_rng branch from a8ea4c0 to 00c8e1a Compare March 26, 2025 12:58

Damian-Nordic reviewed Mar 27, 2025

View reviewed changes

Vge0rge marked this pull request as draft March 28, 2025 14:51

Vge0rge force-pushed the 54h20_psa_rng branch from 00c8e1a to 2814a25 Compare April 1, 2025 12:43

Vge0rge force-pushed the 54h20_psa_rng branch from 2814a25 to 133ff39 Compare April 1, 2025 12:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nrf_rpc: Add function for single group initialization #1593

nrf_rpc: Add function for single group initialization #1593

Vge0rge commented Nov 27, 2024

doki-nordic Dec 10, 2024

Vge0rge Dec 11, 2024

doki-nordic Dec 10, 2024

Vge0rge Dec 11, 2024

Vge0rge Dec 13, 2024

pdunaj Dec 11, 2024

Vge0rge Dec 13, 2024

pdunaj Dec 11, 2024

Vge0rge Dec 13, 2024

pdunaj Dec 11, 2024

Vge0rge Dec 13, 2024

pdunaj Dec 11, 2024

Vge0rge Dec 13, 2024

pdunaj Dec 11, 2024

Vge0rge Dec 13, 2024

pdunaj commented Dec 11, 2024

pdunaj commented Dec 12, 2024

frkv left a comment

Damian-Nordic Mar 27, 2025

Vge0rge Mar 27, 2025

Damian-Nordic Mar 27, 2025

Damian-Nordic Mar 27, 2025

Vge0rge Mar 27, 2025

Damian-Nordic Mar 27, 2025 •

edited

Loading

Vge0rge commented Mar 28, 2025


		NRF_RPC_ASSERT(transport != NULL);
		if(group->data->transport_initialized) {

nrf_rpc: Add function for single group initialization #1593

Are you sure you want to change the base?

nrf_rpc: Add function for single group initialization #1593

Conversation

Vge0rge commented Nov 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdunaj commented Dec 11, 2024

pdunaj commented Dec 12, 2024

frkv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Damian-Nordic Mar 27, 2025 • edited Loading

Choose a reason for hiding this comment

Vge0rge commented Mar 28, 2025

Damian-Nordic Mar 27, 2025 •

edited

Loading